Start using serde_derive in a couple places in the compiler. #56447

eddyb · 2018-12-02T23:02:58Z

Note that this doesn't actually use serde for anything currently - I have a different branch where I started that, but I thought I'd extract this proof of concept out, to deal with build system issues.

The second commit is needed to fix this error in rustc_codegen_llvm and rustdoc:

error[E0463]: can't find crate for `serde_derive` which `rustc` depends on

This problem arises because we use different Cargo build dirs for codegen backends and rustdoc from the main rustc Cargo build dir, and without the second commit in this PR, the host deps (including serde_derive) weren't copied over to the common sysroot (shared between all builds).

Quite surprisingly, everything else seems to "just work"!
This is in part because of pre-existing logic in rustbuild to only use stage0 for build scripts when building libstd itself, which happens to be what we need to make proc macros work in all stages 🎉

cc @alexcrichton @Mark-Simulacrum @Zoxc @nikomatsakis

rust-highfive · 2018-12-02T23:03:01Z

r? @estebank

(rust_highfive has picked a reviewer for you, use r? to override)

eddyb · 2018-12-02T23:11:25Z

r? @alexcrichton

Mark-Simulacrum · 2018-12-02T23:14:40Z

@bors try to check a dist build (though I don't expect it to be different)

bors · 2018-12-02T23:14:51Z

⌛ Trying commit 7b22fcb with merge d934074...

@alexcrichton

Start using serde_derive in a couple places in the compiler. Note that this doesn't actually use `serde` for anything currently - I have a different branch where I started that, but I thought I'd extract this proof of concept out, to deal with build system issues. The second commit is needed to fix this error in `rustc_codegen_llvm` and `rustdoc`: ``` error[E0463]: can't find crate for `serde_derive` which `rustc` depends on ``` This problem arises because we use different Cargo build dirs for codegen backends and `rustdoc` from the main `rustc` Cargo build dir, and without the second commit in this PR, the host `deps` (including `serde_derive`) weren't copied over to the common sysroot (shared between all builds). *Quite surprisingly*, everything else seems to "just work"! This is in part because of pre-existing logic in `rustbuild` to only use `stage0` for build scripts when building libstd itself, which *happens to* be what we need to make proc macros work in all stages 🎉 cc @alexcrichton @Mark-Simulacrum @Zoxc @nikomatsakis

rust-highfive · 2018-12-03T00:36:19Z

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

travis_time:end:21b204be:start=1543791871065679542,finish=1543791926927771954,duration=55862092412
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#Pull-Requests-and-Security-Restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-5.0
---
travis_time:start:test_codegen
Check compiletest suite=codegen mode=codegen (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[00:59:30] 
[00:59:30] running 119 tests
[00:59:33] i..ii...iii..iiii.....i...i.........i..iii.............i......i....ii...i..i.ii..............i...ii. 100/119
[00:59:33] .ii.i.....iiii.....
[00:59:33] 
[00:59:33]  finished in 3.559
[00:59:33] travis_fold:end:test_codegen

---
travis_time:start:test_debuginfo
Check compiletest suite=debuginfo mode=debuginfo-both (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[00:59:48] 
[00:59:48] running 118 tests
[01:00:13] .iiiii...i.....i..i...i..i.i..i.i..i.....i..i....i..........iiii.........i.i....i...i.......ii.i.i.i 100/118
[01:00:17] ......iii.i.....ii
[01:00:17] 
[01:00:17]  finished in 29.105
[01:00:17] travis_fold:end:test_debuginfo

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

bors · 2018-12-03T01:49:25Z

☀️ Test successful - status-travis
State: approved= try=True

… crates, too.

rust-highfive · 2018-12-03T07:45:07Z

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

travis_time:end:33bc9f16:start=1543817953872979004,finish=1543818009818555471,duration=55945576467
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#Pull-Requests-and-Security-Restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-5.0
---
travis_time:start:test_codegen
Check compiletest suite=codegen mode=codegen (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[00:57:00] 
[00:57:00] running 119 tests
[00:57:03] i..ii...iii..iiii.....i...i.........i..iii.............i.....i.....ii...i..i.ii..............i...ii. 100/119
[00:57:04] .ii.i.....iiii.....
[00:57:04] 
[00:57:04]  finished in 3.356
[00:57:04] travis_fold:end:test_codegen

---
travis_time:start:test_debuginfo
Check compiletest suite=debuginfo mode=debuginfo-both (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[00:57:18] 
[00:57:18] running 118 tests
[00:57:41] .iiiii...i.....i..i...i..i.i..i.i..i.....i..i....i..........iiii.........i.i....i...i.......ii.i.i.i 100/118
[00:57:45] ......iii.i.....ii
[00:57:45] 
[00:57:45]  finished in 27.413
[00:57:45] travis_fold:end:test_debuginfo

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

eddyb · 2018-12-03T07:56:24Z

Looks like the last failure is caused by build_helper and cmake being copied, with the rest of the host-specific deps, but they're built by stage0/bin/rustc.

Also, in general, I feel like we should distinguish between "crates you can use from the sysroot" and "dependencies of sysroot crates", similar to how Cargo uses -Ldependency for deps.
Maybe have a directory nested in, or next to, lib/rustlib/$target/lib, for such deps.
That way, people can't just accidentally, say, use extern crate serde; and get an error from rustc_private (even if we provide a useful message, it's still suboptimal).

alexcrichton · 2018-12-03T22:47:25Z

Hm I think I'm a bit lost here, how does this patch work? Shouldn't procedural macros always be compiled by the compiler that build the standard library in that stage? (aka not always the stage0 compiler). I think though that with our shim it's always using the stage0 compiler?

Also.. how does this work without your patches on nightly for decoupling proc_macro/rustc?

eddyb · 2018-12-04T03:31:43Z

@alexcrichton So the helpfully-preexisting behavior I allude in the PR description is here:

rust/src/bootstrap/builder.rs

Lines 959 to 983 in c8ae2de

    
           // Almost all of the crates that we compile as part of the bootstrap may 
        
           // have a build script, including the standard library. To compile a 
        
           // build script, however, it itself needs a standard library! This 
        
           // introduces a bit of a pickle when we're compiling the standard 
        
           // library itself. 
        
           // 
        
           // To work around this we actually end up using the snapshot compiler 
        
           // (stage0) for compiling build scripts of the standard library itself. 
        
           // The stage0 compiler is guaranteed to have a libstd available for use. 
        
           // 
        
           // For other crates, however, we know that we've already got a standard 
        
           // library up and running, so we can use the normal compiler to compile 
        
           // build scripts in that situation. 
        
           // 
        
           // If LLVM support is disabled we need to use the snapshot compiler to compile 
        
           // build scripts, as the new compiler doesn't support executables. 
        
           if mode == Mode::Std || !self.config.llvm_enabled { 
        
               cargo 
        
                   .env("RUSTC_SNAPSHOT", &self.initial_rustc) 
        
                   .env("RUSTC_SNAPSHOT_LIBDIR", self.rustc_snapshot_libdir()); 
        
           } else { 
        
               cargo 
        
                   .env("RUSTC_SNAPSHOT", self.rustc(compiler)) 
        
                   .env("RUSTC_SNAPSHOT_LIBDIR", self.rustc_libdir(compiler)); 
        
           }

That's almost literally the exact same logic I mentioned I was considering implementing on Discord, except someone already did it!
Build scripts can always use stage0/bin/rustc so they don't need it, but it still applies.

As for stage0/bin/rustc not supporting the new decoupling: we can't use stage0/bin/rustc with the locally built src/libproc_macro anyway, while src/libproc_macro is now "ABI-safe", it still needs two copies built from the same source to be compatible (we could even include a version hash in the protocol to ensure we don't accidentally mix them up).

What should happen at stage0 is that proc macros are built against (the downloaded) stage0/lib/rustlib/**/libproc_macro*, not the locally-built version, but higher stages should use the proc_macro they built (from stage1/lib/rustlib).

Note that using proc macros in the compiler has always worked, but only at stage0, my decoupling fixes stage1 onwards (but primarily only stage1).

This might be accidentally right in rustbuild, again, because the locally-built libraries of stage0 are in stage0-sysroot/lib/rustlib, while stage0 contains the downloaded versions.
But stageN/lib/rustlib where N > 0 contains locally-built-by-stageN libraries, so what we need happens to be in stageN/lib/rustlib for all N.

To be really honest, I wasn't expecting this to work either without more tweaks. But someone from the past was too clever in rustbuild and stumbled upon what we need for proc macros!

alexcrichton · 2018-12-04T16:10:41Z

Ok this is starting to make a bit more sense I think, we'll always compile procedural macros against the stage0 proc_macro crate, and it's the proc_macro crate freshly compiled.

What I still don't think I understand though is how this works at all in stage0 right now. Currently our stage0 compiler does the old procedural macro system. This PR, however, compiles serde_derive in stage0 against the stage0 proc_macro crate previously built. That proc_macro crate is wildly incompatible with the bootstrap compiler which we're compiling with, right? How does that get hooked up correctly?

As a side note as well, does this mean that we can never change the ABI of procedural macros? If procedural macros are always compiled against the stage0 version, how do we change the signatures?

alexcrichton · 2018-12-04T16:11:17Z

src/bootstrap/compile.rs

            // If this was output in the `deps` dir then this is a precise file
            // name (hash included) so we start tracking it.
-            if filename.starts_with(&target_deps_dir) {
+            if filename.starts_with(&host_root_dir) || filename.starts_with(&target_deps_dir) {


Can you expand a bit on what's going on here? This feels like it shouldn't be necessary...

The PR description tries to explain this. It's needed for the compiler to be able to find the (host-only) proc macro dependencies of e.g. librustc, from across a Cargo build (i.e. from codegen backends).

Hm ok, I ask because I don't think that this is right. This I think is definitely not what we want when cross-compiling because it's mixing architectures of libraries in one directory. For non-cross-compiling situations this'll copy too much for things like build scripts and/or intermediate proc-macro dependencies.

Could we perhaps whitelist an explicit copy of serde_derive if necessary? Or something like that?

It would be nice if cargo would tell us if artifacts are proc macros, but I think a whitelist will suffice here

Mixing architectures isn't a problem for rustc, and a whitelist feels hackier IMO.

I still like the idea of making the same -Ldependency= distinction Cargo makes, in the sysroot, as well (from #56447 (comment)).

In fact, we can even copy the host deps to rustlib/$host/deps instead of rustlib/$target/deps, and have the compiler look in the right place.

I should also mention I was able to work around this by adding a serde_derive dependency to librustc_codegen_llvm, but I feel like this also doesn't scale (and it builds serde_derive more than once).

I guess I'd accept the whitelisting if someone else implemented it, just to unblock @Zoxc.

bors · 2018-12-08T06:18:32Z

☔ The latest upstream changes (presumably #56578) made this pull request unmergeable. Please resolve the merge conflicts.

alexcrichton · 2018-12-12T16:54:07Z

@eddyb do you perhaps want to take the strategy from #56462 to rebase and land this?

Dylan-DPC-zz · 2019-01-14T17:12:36Z

ping from triage @eddyb you need to rebase this

TimNN · 2019-01-22T18:51:21Z

Ping from triage @eddyb: What is the status of this PR?

Dylan-DPC-zz · 2019-02-04T14:17:56Z

ping from triage @eddyb Unfortunately we haven't heard from you on this in a while, so I'm closing the PR to keep things tidy. Don't worry though, if you'll have time again in the future please reopen this PR, we'll be happy to review it again!

eddyb added 2 commits December 3, 2018 00:50

Start using serde_derive in a couple places in the compiler.

81ac104

bootstrap: include host deps, not just target ones, in run_cargo.

7b22fcb

rust-highfive assigned estebank Dec 2, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 2, 2018

rust-highfive assigned alexcrichton and unassigned estebank Dec 2, 2018

bootstrap: respect RUSTC_FORCE_UNSTABLE for build script/proc macro…

eb1bca5

… crates, too.

eddyb mentioned this pull request Dec 3, 2018

Add a method to the arena to promote objects to it if they implement LocalDrop #56448

Closed

eddyb mentioned this pull request Dec 4, 2018

[WIP] Move from rustc_serialize::json to serde_json. #56511

Closed

alexcrichton reviewed Dec 4, 2018

View reviewed changes

alexcrichton mentioned this pull request Dec 9, 2018

Define queries using a proc macro #56462

Merged

Zoxc mentioned this pull request Dec 13, 2018

Start using serde_derive in a couple places in the compiler #56795

Closed

Dylan-DPC-zz added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 14, 2019

TimNN added A-allocators Area: Custom and system allocators and removed A-allocators Area: Custom and system allocators labels Jan 29, 2019

Dylan-DPC-zz closed this Feb 4, 2019

Dylan-DPC-zz added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 4, 2019

Zoxc mentioned this pull request Oct 29, 2019

Dual proc macro hash #65698

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Start using serde_derive in a couple places in the compiler. #56447

Start using serde_derive in a couple places in the compiler. #56447

eddyb commented Dec 2, 2018

rust-highfive commented Dec 2, 2018

eddyb commented Dec 2, 2018

Mark-Simulacrum commented Dec 2, 2018

bors commented Dec 2, 2018

rust-highfive commented Dec 3, 2018

bors commented Dec 3, 2018

rust-highfive commented Dec 3, 2018

eddyb commented Dec 3, 2018 •

edited

Loading

alexcrichton commented Dec 3, 2018

eddyb commented Dec 4, 2018 •

edited

Loading

alexcrichton commented Dec 4, 2018

alexcrichton Dec 4, 2018

eddyb Dec 4, 2018

alexcrichton Dec 7, 2018

Zoxc Dec 7, 2018

eddyb Dec 7, 2018

bors commented Dec 8, 2018

alexcrichton commented Dec 12, 2018

Dylan-DPC-zz commented Jan 14, 2019 •

edited

Loading

TimNN commented Jan 22, 2019

Dylan-DPC-zz commented Feb 4, 2019

Start using serde_derive in a couple places in the compiler. #56447

Start using serde_derive in a couple places in the compiler. #56447

Conversation

eddyb commented Dec 2, 2018

rust-highfive commented Dec 2, 2018

eddyb commented Dec 2, 2018

Mark-Simulacrum commented Dec 2, 2018

bors commented Dec 2, 2018

rust-highfive commented Dec 3, 2018

bors commented Dec 3, 2018

rust-highfive commented Dec 3, 2018

eddyb commented Dec 3, 2018 • edited Loading

alexcrichton commented Dec 3, 2018

eddyb commented Dec 4, 2018 • edited Loading

alexcrichton commented Dec 4, 2018

alexcrichton Dec 4, 2018

Choose a reason for hiding this comment

eddyb Dec 4, 2018

Choose a reason for hiding this comment

alexcrichton Dec 7, 2018

Choose a reason for hiding this comment

Zoxc Dec 7, 2018

Choose a reason for hiding this comment

eddyb Dec 7, 2018

Choose a reason for hiding this comment

bors commented Dec 8, 2018

alexcrichton commented Dec 12, 2018

Dylan-DPC-zz commented Jan 14, 2019 • edited Loading

TimNN commented Jan 22, 2019

Dylan-DPC-zz commented Feb 4, 2019

eddyb commented Dec 3, 2018 •

edited

Loading

eddyb commented Dec 4, 2018 •

edited

Loading

Dylan-DPC-zz commented Jan 14, 2019 •

edited

Loading